Skip to content

Support PackageImports in hiddenPackageSuggestion #4537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Apr 3, 2025
Merged

Support PackageImports in hiddenPackageSuggestion #4537

merged 2 commits into from
Apr 3, 2025

Conversation

@jian-lin
Copy link
Collaborator

@jian-lin jian-lin commented Apr 2, 2025 •
edited
Loading

Fix: #4479

TODO: Probably I need to add more tests.

CC @VeryMilkyJoe @fendor

Video demo

hls-4479-demo.mp4

More info

Here is the string being matched.

With PackageImports

not found: Could not find module ‘Data.List.Split’
Perhaps you meant
  Data.List.Split (needs flag -package-id split-0.2.5)
  Data.List.Split (needs flag -package-id split-0.2.5)

Without PackageImports

not found: Could not load module ‘Data.List.Split’
It is a member of the hidden package ‘split-0.2.5’.
Perhaps you need to add ‘split’ to the build-depends in your .cabal file.
It is a member of the hidden package ‘split-0.2.5’.
Perhaps you need to add ‘split’ to the build-depends in your .cabal file.

I also notice that the "not found" error remains even after the dependency is added by the code action (with and without PackageImports). That may be a bug of my lsp client (eglot on Emacs) or HLS. Needs further investigation.

@jian-lin jian-lin requested a review from fendor as a code owner April 2, 2025 18:20
@fendor fendor requested a review from VeryMilkyJoe April 3, 2025 12:14
fendor
fendor requested changes Apr 3, 2025
View reviewed changes
@jian-lin jian-lin force-pushed the pr/cabal-add-package-imports branch from 48e4cc5 to 39d3f61 Compare April 3, 2025 16:46
@jian-lin jian-lin requested a review from fendor April 3, 2025 16:48
jian-lin
jian-lin commented Apr 3, 2025
View reviewed changes
Copy link
Collaborator Author

@jian-lin jian-lin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have addressed all previous comments. Please take another look.

fendor
fendor approved these changes Apr 3, 2025
View reviewed changes
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for your contribution ❤️

@jian-lin
Copy link
Collaborator Author

jian-lin commented Apr 3, 2025

Thanks for your review! I had a great experience in my first HLS PR.

@fendor fendor enabled auto-merge (squash) April 3, 2025 17:12
jian-lin
jian-lin commented Apr 3, 2025
View reviewed changes
plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal/CabalAdd.hs
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also notice that the "not found" error remains even after the dependency is added by the code action (with and without PackageImports). That may be a bug of my lsp client (eglot on Emacs) or HLS. Needs further investigation.

Oh, it is indeed a known HLS bug. With HLS 2.10.0.0 and vscode 2.5.3, I need to manually restart HLS or saving hie.yaml after adding dependencies to .cabal file to let HLS find the newly added dependencies.

In addition, there seems to be a bug in my preferred setting: Emacs with eglot as a LSP client. In that setting, only manual restart works and saving hie.yaml has no effect.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that sounds correct!

@fendor fendor disabled auto-merge April 3, 2025 19:46
@fendor
Copy link
Collaborator

fendor commented Apr 3, 2025

@jian-lin It looks like you need to rebase your PR or merge changes from master into it before we can merge :)

@jian-lin jian-lin force-pushed the pr/cabal-add-package-imports branch from 39d3f61 to 4efa5d7 Compare April 3, 2025 19:50
@jian-lin
Copy link
Collaborator Author

jian-lin commented Apr 3, 2025

@fendor Rebased.

@fendor fendor enabled auto-merge (squash) April 3, 2025 19:53
VeryMilkyJoe
VeryMilkyJoe approved these changes Apr 3, 2025
View reviewed changes
Copy link
Collaborator

@VeryMilkyJoe VeryMilkyJoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@fendor fendor merged commit 42a5261 into haskell:master Apr 3, 2025
41 checks passed
@jian-lin jian-lin deleted the pr/cabal-add-package-imports branch April 3, 2025 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fendor fendor fendor approved these changes

@VeryMilkyJoe VeryMilkyJoe VeryMilkyJoe approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add Package doesn't work with package-imports

3 participants

@jian-lin @fendor @VeryMilkyJoe